cc @mcollina @ronag

I think llhttp should abstract away the wasm instantiation, callback tracking, reading strings etc. maybe something like:

const llhttp = require('llhttp/wasm');

const parser = new llhttp.Parser(llhttp.RESPONSE, {
  onX() {},
  onY() {},
})

parser.execute(bytes); // might throw instead of returning an errno

You could also export llhttp.wasmModule so people can create their own instances.

Creating our instance is what we've done over at nodejs/undici#575. Where we on purpose only have implemented RESPONSE. Would it be sufficient to just do llhttp.wasmModule for now?

hmm I was thinking the logic would still be implemented by the user, like they'd put an llhttp.Parser inside their HTTPParser (like how node uses llhttp_t inside its HTTPParser). the interface on the llhttp side would just be responsible for converting errors and strings and keeping the callbacks consistent. I'm not maintaining this though so you do you.

Also a sidenote, there's a useful crossover between versions of node that expose the internal http parser, and versions of node that support FinalizationRegistry, so I think you could hardcode gc on this side.

hmm I was thinking the logic would still be implemented by the user, like they'd put an llhttp.Parser inside their HTTPParser (like how node uses llhttp_t inside its HTTPParser). the interface on the llhttp side would just be responsible for converting errors and strings and keeping the callbacks consistent. I'm not maintaining this though so you do you.

Also a sidenote, there's a useful crossover between versions of node that expose the internal http parser, and versions of node that support FinalizationRegistry, so I think you could hardcode gc on this side.

I like this idea. Given that we are still in an early stage, as @ronag said even just a wasm module is enough for now if it's not a problem of course. There's a lot of room for improvement. I would be ok with merging this in a custom branch if it's better.

It really depends what @indutny prefers :).

It really depends what @indutny prefers :).

Yes, of course

Yay. WASM!

I think we should strive for a saner API like @devsnek has posted, but having raw WASM support as an unstable feature is acceptable too. Not locking into JS API means that you'd have more room to experiment with it an undici, right?

Not locking into JS API means that you'd have more room to experiment with it an undici, right?

Yes, exactly.

I think the JS API will organically define itself while we experiment (I would be happy to try to implement it). Defining a JS API here would also mean that we would have to install llhttp as a dependency in undici, and we would like to avoid any dependencies (formally declared in package.json) atm.

Just for reference:

It would be nice if the wasm build lived in undici. That way we could apply undici specific optimizations in the wasm api and reduce the number of wasm->js calls. I think we could move more of the HTTPParser logic into native/wasm land.
e.g. wasm_on_header_field and wasm_on_header_value might be possible to combine into wasm_on_headers.

i.e. if we put wasm in llhttp we might want to do some semver major (?) changes in the future related to optimizations of the API. If that makes sense?

So is this ready for review?

Yes it is.

@indutny any idea when you might have time to review this?

This looks very good, but I've left few nitpicks/requests that have to be resolved before merging. Thank you for submitting it, and sorry for a long delay on my end.

No worries, thank you for the review. I'll work on it!

To reiterate:

these changes add some scripts to build wasm. Nothing is exposed in the package itself. If one wanted to use the build, they would have to copy the build/wasm folder in their project and write their own parser.

I see a lot of potential with wasm. The next step would be exposing a set of API at the package level that allows the user to install llhttp as a dependency and do something like:

import { createParser, constants } from 'llhttp';

const parser = createParser(constants.TYPE.RESPONSE, {
  ...// other options
})

As you already suggested.

Released in v5.0.0